Improved file ordering in session - #41
Merged
Merged
Conversation
chris-adam
marked this pull request as ready for review
May 27, 2026 11:16
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds ChangesItem Ordering Adapter Pattern
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant SessionInsertion
participant ItemOrderAdapter
participant Context
SessionInsertion->>ItemOrderAdapter: Request item order
ItemOrderAdapter->>Context: Enumerate items and read UIDs
Context-->>ItemOrderAdapter: Ordered items
ItemOrderAdapter-->>SessionInsertion: Return UID-to-index mapping
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/imio/esign/tests/test_adapters.py (1)
31-33: ⚡ Quick winAssert complete UID→index mapping equality.
This loop validates expected keys but not unexpected extra entries in
order. Build the expected mapping and assert full dict equality.Proposed test tightening
- children = list(folder.values()) - for idx, child in enumerate(children): - self.assertEqual(order[child.UID()], idx) + children = list(folder.values()) + expected_order = {child.UID(): idx for idx, child in enumerate(children)} + self.assertEqual(order, expected_order)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imio/esign/tests/test_adapters.py` around lines 31 - 33, The test currently loops over children and asserts each child's UID maps to the expected index but doesn't catch extra keys in order; instead, construct an expected mapping from the actual children (e.g., expected = {child.UID(): idx for idx, child in enumerate(list(folder.values()))}) and then assert equality between expected and order (use assertEqual(expected, order)) so the test fails on both missing and unexpected entries; update the code around the loop that references children, folder.values(), order, and child.UID() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/imio/esign/tests/test_adapters.py`:
- Around line 31-33: The test currently loops over children and asserts each
child's UID maps to the expected index but doesn't catch extra keys in order;
instead, construct an expected mapping from the actual children (e.g., expected
= {child.UID(): idx for idx, child in enumerate(list(folder.values()))}) and
then assert equality between expected and order (use assertEqual(expected,
order)) so the test fails on both missing and unexpected entries; update the
code around the loop that references children, folder.values(), order, and
child.UID() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40137db8-1a6a-45cf-9565-5de5da02c4d1
📒 Files selected for processing (6)
CHANGES.rstsrc/imio/esign/adapters.pysrc/imio/esign/configure.zcmlsrc/imio/esign/interfaces.pysrc/imio/esign/tests/test_adapters.pysrc/imio/esign/utils.py
Coverage Report for CI Build 32146829192Coverage increased (+0.1%) to 86.842%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
gbastien
approved these changes
Aug 18, 2026
gbastien
left a comment
gbastien
left a comment
Member
There was a problem hiding this comment.
LGTM
Salut @chris-adam j'ai testé et surchargé pour afficher l'annexe principale générée puis les annexes "à signer" du point et çà fonctionne bien.
Juste en passant, je vois que tu ajoutes un test pour get_context_uid qui existait déjà mais pas testé, en lisant l'implémentation de get_context_uid çà m'a perturbé car très défensif et finalement pas nécessaire :-) Je ferais comme implémentation de get_context_uid: "return self.context.aq_parent.UID()" car c'est une annexe et dans Zope tu as d'office aq_parent et d'office aussi la fonction UID...
Pour moi çà peut être mergé (je vais merger le main pour màj la branche)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests